feat(ui): implement predictive back gesture and migrate Key.Back to BackHandler - #498
Conversation
5e9f121 to
fc777c6
Compare
…Selector - Create ArvioPredictiveBack motion primitive (M3 surface scale 90%, edge-aware shift, touchY tilt, and instant release handling) - Apply ArvioBackSurface, ArvioBackPeek, and AnimatedVisibility exit transitions to SettingsScreen sub-pages - Apply ArvioBackModal and smooth fade/slide-down exit transitions to StreamSelector modal overlay
…ctive back gestures
f2f5bc1 to
d65e5ff
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5df9fb5c05
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| android:allowBackup="false" | ||
| android:banner="@drawable/app_banner" | ||
| android:enableOnBackInvokedCallback="false" | ||
| android:enableOnBackInvokedCallback="true" |
There was a problem hiding this comment.
With this application-wide flag enabled on API 33+ / targetSdk 36, system Back is delivered through the OnBackInvoked/OnBackPressed path instead of the Compose key-event path, but a repo-wide check still shows screens and modals that only handle Key.Back in onKeyEvent/onPreviewKeyEvent and have no BackHandler in those files, e.g. app/src/main/kotlin/com/arflix/tv/ui/components/TextInputModal.kt:142 and app/src/main/kotlin/com/arflix/tv/ui/screens/plugin/PluginScreen.kt:130. In those contexts Back will no longer dismiss/navigate as intended and can fall through to the activity/root back behavior, so either keep this flag off or migrate the remaining Key.Back handlers to BackHandler before enabling it globally.
Useful? React with 👍 / 👎.
ProdigyV21
left a comment
ProdigyV21
left a comment
There was a problem hiding this comment.
Thanks Himanth — ArvioPredictiveBack.kt is genuinely good work: M3-spec easing, real commit/cancel settle animations, and the CancellationException rethrow is handled correctly. The StreamSelector and mobile Settings integrations look right. This is closer to mergeable than #495, but there's one blocker that makes it dangerous to land as-is.
Blocker: the manifest flag is flipped app-wide, but the migration it requires isn't in this PR.
The description says "App-Wide Key.Back → BackHandler Migration (25 Files)" — the diff migrates exactly one (StreamSelector). Checked against the branch:
- The codebase has 55
Key.Backinterceptions across 25 files. After this PR, 54 remain: PlayerScreen keeps 7, the TV Settings layout keeps 14, and 17 files — every context menu,AudioTrackSelector,PersonModal,TextInputModal,QuickZapOverlay,SearchOverlay,NextEpisodeOverlay,DetailsScreen,EpgGrid, and more — haveKey.Backas their only back mechanism, with noBackHandlerat all. - With
enableOnBackInvokedCallback="true"andtargetSdk 36, Android 13+ stops deliveringKEYCODE_BACKto the app's views entirely — that's the documented contract of opting in. Every remaining interceptor goes silently dead on Android 13+. - Concrete failure: on a Google TV Streamer (Android 14), Back with a context menu open no longer closes the menu — it fires whatever
BackHandlersits underneath (or exits the screen) while the menu stays on screen. Same for the audio-track selector during playback, the person modal, text input, quick-zap… Back is the navigation primitive on TV, and this forks its behavior by OS version: fine on Android ≤ 12, broken on 13+.
Note the risk/benefit inversion: predictive back gestures don't exist on TV (d-pad Back commits with no progress events), so the flag's visible benefit is mobile-only — while the breakage lands on the TV form factor.
What would make this mergeable — either path works:
- Do the migration the description promises: move all 55
Key.Backsites toBackHandler/PredictiveBackHandlerwith correctenabledstate and modal ownership (topmost layer wins), then flip the flag — and test on an actual Android 13+ device, since ≤ 12 can't reveal any of these regressions. - Ship the motion work now, defer the flag: keep
enableOnBackInvokedCallback="false", landArvioPredictiveBack.kt+ the StreamSelector/Settings integrations (they work fine via the compat dispatcher), and flip the flag in a follow-up once the migration is complete.
Option 2 gets your animation work in quickly and safely.
Also please split out the TvLazyColumn → LazyColumn migration in StreamSelector. It changes d-pad focus/scroll behavior in the source picker, it's unrelated to predictive back, and it deserves its own PR where it can be reviewed and tested as what it is.
|
Thanks @ProdigyV21! Addressed all review feedback in the latest commit () on : 1. Completed App-Wide Migration (Option 1)
2. Isolated in
3. Verification
|
github-actions
Bot
added
the
area: android
2 participants
Summary
This PR implements predictive back gesture support for the Stream Searching page (
StreamSelector.kt) and refactors app-wide back navigation handling to ensure compatibility with Android 13+ (OnBackPressedDispatcher).Key Changes
Predictive Back Support (
StreamSelector.kt&AndroidManifest.xml)android:enableOnBackInvokedCallback="true"inAndroidManifest.xml.PredictiveBackHandlerinStreamSelector.ktwith top-level progress tracking.StreamSelectorroot layout into an outer scrim Box (animating dark overlay alpha) and an inner content Box (animating scale, translation, and alpha viagraphicsLayer).CancellationException.App-Wide
Key.Back->BackHandlerMigration (25 Files)Key.Backinterceptions withBackHandleracross all screens, overlays, and modals to prevent back navigation from breaking whenenableOnBackInvokedCallback="true"is active on API 33+.Key.Escapein key listeners for hardware keyboard and desktop users.